Skip to content

Preserve implicits in Quotes context #23263

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

joroKr21
Copy link
Member

@joroKr21 joroKr21 commented May 25, 2025

Fixes #22260

@joroKr21 joroKr21 marked this pull request as ready for review May 25, 2025 14:26
@joroKr21 joroKr21 force-pushed the bugfix/quoted-implicits branch 2 times, most recently from a32c7a2 to 47cd9ba Compare May 25, 2025 14:38
@joroKr21 joroKr21 force-pushed the bugfix/quoted-implicits branch from 47cd9ba to ccc5d1a Compare May 26, 2025 05:51
@joroKr21 joroKr21 requested a review from jchyb May 26, 2025 10:15
Copy link
Contributor

@jchyb jchyb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a very good PR, thank you so much @joroKr21 for fixing this and signaling the issue, especially with that second minimisation. Since this is a big change I was worried if any regressions across the ecosystem would pop up, however the open community build did not seem to find anything, so I am mostly comfortable with merging. I do have one question about something.

Also apologies for taking a long time on this, I was unavailable until recently.

else if info.isImplicitMethod then Implicit | SyntheticParam
else SyntheticParam
val params = info.paramNames.lazyZip(info.paramInfos).map: (pname, ptype) =>
val flags = if ptype.hasAnnotation(defn.ErasedParamAnnot) then commonFlags | Erased else commonFlags
Copy link
Contributor

@jchyb jchyb Jun 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't figure this part out - adding erased flags to the param flags seems like it might a good change, but I don't see how it relates to the rest of the PR. Is it necessary? I tried running the compilation test without this and it seemed fine (I am only talking about the Erased flag here).

Copy link
Member Author

@joroKr21 joroKr21 Jun 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The JS tests failed without it: https://github.com/scala/scala3/actions/runs/15238955325/job/42856571339 - I guess this was caused by the discrepancy of the parameter symbols in the tree vs those in the type signature. Which are now the same in this PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. Thank you!

Copy link
Contributor

@jchyb jchyb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll merge this tomorrow morning. Thank you once again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Symbol.asQuotes doesn't populate the implicit scope
2 participants